Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tender locations and addresses, make item locations and addresses plural #34

Merged
merged 5 commits into from
Mar 11, 2022

Conversation

duncandewhurst
Copy link
Member

Deprecate Item.deliveryAddress and Item.deliveryLocation in favor of 
Item.deliveryAddresses and Item.deliveryLocations respectively
@duncandewhurst duncandewhurst changed the title 173 tender location Add tender locations and addresses, make item locations and addresses plural Dec 9, 2021
@duncandewhurst
Copy link
Member Author

test_json.py reports some failures unrelated to this PR:

 test_json.py::test_schema_valid[/home/runner/work/ocds_location_extension/ocds_location_extension/release-schema.json-release-schema.json-data0]
  release-schema.json has "properties" within "properties" at /definitions/Location/properties/geometry

test_json.py::test_schema_valid[/home/runner/work/ocds_location_extension/ocds_location_extension/release-schema.json-release-schema.json-data0]
  release-schema.json has "properties" within "properties" at /definitions/Location/properties/gazetteer

and some failures relating to missing ids:

 test_json.py::test_json_merge_patch
  release-schema.json is missing "id" in "items/properties" at /definitions/

However, based on the logic in https://ocds-standard-development-handbook.readthedocs.io/en/latest/meta/schema_style_guide.html#id-field I'm not sure that an id field is required for addresses or locations, although we did add an id field to Location in OC4IDS.

@duncandewhurst
Copy link
Member Author

@jpmckinney please can you add @odscrachel and @mrshll1001 to the team for the open-contracting-extensions organisation so that I can request a review from them?

@jpmckinney
Copy link
Member

jpmckinney commented Dec 9, 2021

They have now been invited. I'll look into the tests and add exceptions as needed.

@duncandewhurst
Copy link
Member Author

Thanks!

@jpmckinney
Copy link
Member

@duncandewhurst Please set "wholeListMerge": true for those arrays, to make the merge behavior explicit. Feel free to add this clarification to the handbook as well.

Copy link

@mrshll1001 mrshll1001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good

@yolile
Copy link
Contributor

yolile commented Mar 11, 2022

@jpmckinney I think this is ready to be merged?

@jpmckinney
Copy link
Member

Yes, somehow fell out of my inbox.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discussion: Tender location directly tied to tender
4 participants